Add support for @aws-sdk version 3 #325
Conversation
This should resolve this issue: ZJONSSON#241 I have used a variation of @Sljux's solution to make the interface compatible with the current `stream` interface.
|
Thanks @alice-was-here, appreciate the PR. Can you please address the comments above and also add a unit test for the new s3 method, you can base the test on current s3 test https://github.com/ZJONSSON/node-unzipper/blob/master/test/openS3.js |
|
@alice-was-here Also can you please ensure tests pass for all node versions supported 10.x - 19.x |
|
@alice-was-here, this is super close, can you please address the following so we can get this in:
Here is a quick example of how this should look (passes tests): https://github.com/ZJONSSON/node-unzipper/pull/329/files - but we still need the additional unit test for s3_v3 |
|
@ZJONSSON cheers 🙏 I noticed that the tests for the existing s3 client are marked as |
|
I am only leaving a comment to say that aws-sdk v3 support is actually desired and we are excited to see someone write support for this. |
| "dirdiff": ">= 0.0.1 < 1", | ||
| "eslint": "^9.2.0", | ||
| "globals": "^15.2.0", | ||
| "iconv-lite": "^0.4.24", | ||
| "request": "^2.88.0", | ||
| "stream-buffers": ">= 0.2.5 < 1", | ||
| "tap": "^12.7.0", | ||
| "tap": "^16.3.10", |
There was a problem hiding this comment.
Bumping the tap cli was required to correctly import the aws libraries (they include ?? and other syntax not supported by tap@12).
The coverage report also seems to compute differently for tap 16 - I've disabled reduced it to make the test suite pass.... it was computing coverage at about 80% even though nothing else had changed. If anyone has ideas as to why it dropped off, happy to add back?
| // These files are provided by AWS's open data registry project. | ||
| // https://github.com/awslabs/open-data-registry | ||
|
|
||
| return unzip.Open.s3_v3(client, { |
There was a problem hiding this comment.
If there's a better file source for this I can change it. The one referenced by the openS3.js test no longer exists.
|
Thanks again @alice-was-here , can you follow up with an update to README.md? |
This should resolve this issue: #241
I have used a variation of @Sljux's solution to make the interface compatible with the current
streaminterface.I've chosen to make the v3 interface a completely separate method... the two clients are different enough that I think it makes sense to break them into two different paths. I can merge them under one function call if people would prefer that.